Skip to content
This repository was archived by the owner on Jul 22, 2024. It is now read-only.

Implements latest Settings spec#1423

Merged
MortimerGoro merged 4 commits into
masterfrom
settings_updates
Jul 24, 2019
Merged

Implements latest Settings spec#1423
MortimerGoro merged 4 commits into
masterfrom
settings_updates

Conversation

@keianhzo

@keianhzo keianhzo commented Jul 18, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #1372 #962 #1440 This PR implements the latests Settings spec which also includes the new user agent switch button in the navigation bar.

@philip-lamb

philip-lamb commented Jul 18, 2019

Copy link
Copy Markdown
Contributor

Awesome. Will give it a test tomorrow.

@keianhzo keianhzo force-pushed the settings_updates branch 2 times, most recently from fc993e3 to 9ec17a9 Compare July 18, 2019 15:14
MortimerGoro
MortimerGoro suggested changes Jul 19, 2019
View reviewed changes

@MortimerGoro MortimerGoro left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I have tried to switch between desktop and VR UAs. It works on the current page, but when browsing to other pages I got a desktop viewport while the VR icon was visible in the URL bar

  • I think the VR text icon can be confusing for users. Can't we use a similar icon to the one used in Oculus browser? Toggle between the desktop icon with or without the checkmark.

  • The VR icon is too close to the URL bar end on the content feed and looks misaligned. Should we hide it on the content feed? (in that case the bookmark icon is not visible)

@MortimerGoro

MortimerGoro commented Jul 19, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

cc @jvonitter @thenadj related to the checkmark idea I mentioned:

Firefox desktop uses this icon to enable Mobile/Responsive layout. It changes color when enabled/disabled.
Captura de pantalla 2019-07-19 a las 11 53 18

Firefox for iOS uses this icon to go back to Mobile/Responsive layout once desktop is enabled. It shows/hides the blue check when enabled/disabled
Captura de pantalla 2019-07-19 a las 11 58 51

@bluemarvin

bluemarvin commented Jul 19, 2019

Copy link
Copy Markdown
Contributor

I'm not sure if it is related but the first time I ran this PR only the main window was visible, no other UI was visible. I had to restart to see the rest of the UI like navigation bar and tray.

@bluemarvin

bluemarvin commented Jul 19, 2019

Copy link
Copy Markdown
Contributor

Also, I really do not like the VR in the navigation bar. It looks very heavy and it's purpose is not immediately obvious.

@keianhzo keianhzo mentioned this pull request Jul 23, 2019
Closed
@jvonitter

jvonitter commented Jul 23, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

@keianhzo new icons!
UA_Icons.zip

@keianhzo keianhzo mentioned this pull request Jul 24, 2019
Merged
@MortimerGoro MortimerGoro merged commit 795ba86 into master Jul 24, 2019
@bluemarvin bluemarvin deleted the settings_updates branch July 26, 2019 22:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@bluemarvin bluemarvin Awaiting requested review from bluemarvin

1 more reviewer

@MortimerGoro MortimerGoro MortimerGoro approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Settings IA and verbiage updates

5 participants

@keianhzo @philip-lamb @MortimerGoro @bluemarvin @jvonitter